Bugfix for fmt::printf on Power9 architecture with the XL compiler #3256

kennyweiss · 2022-12-31T00:50:50Z

I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.

This PR works around a bug encountered with the IBM xl compiler on the Power9 architecture (e.g. on the Lassen supercomputer ) with fmt::printf().

The issue appears to be that XL is not copying the format_specs struct in the printf_arg_formatter constructor to the arg_formatter base class.

fmt/include/fmt/printf.h

Lines 237 to 238 in a73a9b6

    
           printf_arg_formatter(OutputIt iter, format_specs<Char>& s, context_type& ctx) 
        
               : base{iter, s, locale_ref()}, context_(ctx) {}

This was causing the printf tests to segfault. E.g.

fmt/test/printf-test.cc

Line 73 in a73a9b6

EXPECT_EQ("42", test_sprintf("%1$d", 42));

I resolved the problem by adding an explicit constructor to the arg_formatter base class.

This regression appears to have been introduced between [email protected] and [email protected].

I tested this using the following compiler/config options

set(CMAKE_C_COMPILER "/usr/tce/packages/xl/xl-2022.08.19/bin/xlc" CACHE PATH "")
set(CMAKE_CXX_COMPILER "/usr/tce/packages/xl/xl-2022.08.19/bin/xlC" CACHE PATH "")
set(CMAKE_C_FLAGS "--gcc-toolchain=/usr/tce/packages/gcc/gcc-8.3.1" CACHE STRING "")
set(CMAKE_CXX_FLAGS "--gcc-toolchain=/usr/tce/packages/gcc/gcc-8.3.1" CACHE STRING "")

set(CMAKE_CXX_STANDARD "14" CACHE STRING "")
set(CMAKE_BUILD_TYPE "Debug" CACHE STRING "")
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Wl,-rpath,/usr/tce/packages/xl/xl-2022.08.19/lib" CACHE STRING "Adds a missing rpath for libraries associated with the fortran compiler")

set(FMT_HEADER_ONLY ON CACHE BOOL "")
set(FMT_TEST ON CACHE BOOL "")

Note: There are a few other failing tests with this compiler. I will post separate issues for them.

vitaut

Thanks for the PR but looks like it doesn't work in some CI configs. Also since this is a workaround for printf it's better putting it there (i.e. initialize members in printf_arg_formatter's ctor).

kennyweiss · 2023-01-03T20:40:22Z

Thanks @vitaut.

Since the specs member of the arg_formatter base class is a reference, I couldn't find a way to initialize it in printf_arg_formatter other than through a constructor in the base class. If you know of a better way, I'd be very interested in hearing about it.

(I think) I resolved the error with gcc-4.8 by adding a second constructor that does not require the temporary default locale_ref.

Upstreamed to fmt: fmtlib/fmt#3256

vitaut · 2023-01-04T00:49:30Z

Please submit a bug report to IBM and also could you post the error here for future reference?

kennyweiss · 2023-01-04T04:01:28Z

Thanks @vitaut -- I've created a simple reproducer and will submit to IBM.

Description of the error:

The code in fmt@master compiles with XLC, but segfaults when running the printf unit tests.

In case it helps, here's the stacktrace from gdb:

>gdb ./bin/printf-test

GNU gdb (GDB) Red Hat Enterprise Linux 7.6.1-120.el7
Copyright (C) 2013 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "ppc64le-redhat-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from <fmt>/build-xl/bin/printf-test...done.
(gdb) run
Starting program: <fmt>/build-xl/bin/printf-test
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
[==========] Running 38 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 38 tests from printf_test
[ RUN      ] printf_test.no_args
[       OK ] printf_test.no_args (0 ms)
[ RUN      ] printf_test.escape
[       OK ] printf_test.escape (0 ms)
[ RUN      ] printf_test.positional_args

Program received signal SIGSEGV, Segmentation fault.
0x0000200000178e3c in __dynamic_cast () from /lib64/libstdc++.so.6
Missing separate debuginfos, use: debuginfo-install libgcc-4.8.5-44.el7.ppc64le libstdc++-4.8.5-44.el7.ppc64le
(gdb) bt
#0  0x0000200000178e3c in __dynamic_cast () from /lib64/libstdc++.so.6
#1  0x0000000010195c14 in std::has_facet<fmt::v9::format_facet<std::locale> > (__loc=...) at /usr/tce/packages/gcc/gcc-8.3.1/rh/usr/bin/../lib/gcc/ppc64le-redhat-linux/8/../../../../include/c++/8/bits/locale_classes.tcc:110
#2  0x00000000101a14e8 in fmt::v9::detail::write_loc (out=..., value=..., specs=..., loc=...) at <fmt>/include/fmt/format-inl.h:126
#3  0x000000001007a218 in fmt::v9::detail::write<char, fmt::v9::appender, int, 0> (out=..., value=42, specs=..., loc=...) at <fmt>/include/fmt/format.h:2170
#4  0x000000001007a3ec in fmt::v9::detail::arg_formatter<char>::operator()<int> (this=0x7fffffffad80, value=42) at <fmt>/include/fmt/format.h:3588
#5  0x000000001007a4b0 in fmt::v9::detail::printf_arg_formatter<fmt::v9::appender, char>::operator()<int, 0> (this=0x7fffffffad80, value=42) at <fmt>/include/fmt/printf.h:261
#6  0x0000000010090c14 in fmt::v9::visit_format_arg<fmt::v9::detail::printf_arg_formatter<fmt::v9::appender, char>, fmt::v9::basic_printf_context<fmt::v9::appender, char> >(fmt::v9::detail::printf_arg_formatter<fmt::v9::appender, char>&&, fmt::v9::basic_format_arg<fmt::v9::basic_printf_context<fmt::v9::appender, char> > const&) ( vis=<unknown type in <fmt>/build-xl/bin/printf-test, CU 0x0, DIE 0x86298>, arg=...) at <fmt>/include/fmt/core.h:1643
#7  0x00000000100a4b64 in fmt::v9::detail::vprintf<char, fmt::v9::basic_printf_context<fmt::v9::appender, char> > (buf=warning: RTTI symbol not found for class 'fmt::v9::basic_memory_buffer<char, 500ul, std::allocator<char> >' ..., format=..., args=...) at <fmt>/include/fmt/printf.h:512
#8  0x00000000100a4de0 in fmt::v9::vsprintf<fmt::v9::basic_string_view<char>, char> (fmt=..., args=...) at <fmt>/include/fmt/printf.h:559
#9  0x00000000100b66bc in fmt::v9::sprintf<fmt::v9::basic_string_view<char>, int, char> (fmt=..., args=@0x7fffffffb4c0: 42) at <fmt>/include/fmt/printf.h:576
#10 0x00000000100b678c in test_sprintf<int> (format=..., args=@0x7fffffffb4c0: 42) at <fmt>/test/printf-test.cc:41
#11 0x000000001018f124 in printf_test_positional_args_Test::TestBody (this=0x10404990) at <fmt>/test/printf-test.cc:73
#12 0x0000000010225680 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void> (object=0x10404990, method=&virtual testing::Test::TestBody(), location=0x102fd2d4 "the test body" at <fmt>/test/gtest/gmock-gtest-all.cc:4098
#13 0x00000000102257a0 in testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void> (object=0x10404990, method=&virtual testing::Test::TestBody(), location=0x102fd2d4 "the test body") at <fmt>/test/gtest/gmock-gtest-all.cc:4134
#14 0x00000000102d067c in testing::Test::Run (this=0x10404990) at <fmt>/test/gtest/gmock-gtest-all.cc:4173
#15 0x00000000102d09a8 in testing::TestInfo::Run (this=0x10400990) at <fmt>/test/gtest/gmock-gtest-all.cc:4352
#16 0x00000000102d0c38 in testing::TestSuite::Run (this=0x104005b0) at <fmt>/test/gtest/gmock-gtest-all.cc:4506
#17 0x00000000102d1a48 in testing::internal::UnitTestImpl::RunAllTests (this=0x104001c0) at <fmt>/test/gtest/gmock-gtest-all.cc:7346
#18 0x0000000010223330 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x104001c0, method=(bool (UnitTestImpl::*)(UnitTestImpl * const)) 0x102d1590 <testing::internal::UnitTestImpl::RunAllTests()>, location=0x102fd46c "auxiliary test code (environments or event listeners)") at <fmt>/test/gtest/gmock-gtest-all.cc:4098
#19 0x00000000102234d4 in testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x104001c0, method=(bool (UnitTestImpl::*)(UnitTestImpl * const)) 0x102d1590 <testing::internal::UnitTestImpl::RunAllTests()>, location=0x102fd46c "auxiliary test code (environments or event listeners)") at <fmt>/test/gtest/gmock-gtest-all.cc:4134
#20 0x00000000102d20b4 in testing::UnitTest::Run (this=0x103f11e0 <_$STATIC+10160>) at <fmt>/test/gtest/gmock-gtest-all.cc:6929
#21 0x00000000101912e0 in RUN_ALL_TESTS () at <fmt>/test/gtest/./gtest/gtest.h:12393
#22 0x00000000101913b8 in main (argc=1, argv=0x7fffffffc278) at <fmt>/test/test-main.cc:38
(gdb)

vitaut · 2023-01-06T18:29:43Z

include/fmt/format.h

+  arg_formatter(buffer_appender<Char> it, const format_specs<Char>& s)
+      : out(it), specs(s) {}


This constructor is redundant, please remove.

This is a workaround for gcc-4.8.
#3256 (comment)

Ah, missed that.

vitaut · 2023-01-09T19:47:42Z

I wonder if there is a cleaner workaround that doesn't involve introducing redundant ctors. Would using a function that constructs arg_formatter work? Something along the lines of:

  auto make_arg_formatter(OutputIt iter, format_specs<Char>& s) -> arg_formatter {
    return {iter, s, locale_ref()}
  }

  printf_arg_formatter(OutputIt iter, format_specs<Char>& s, context_type& ctx)
      : base(make_arg_formatter(iter, s), context_(ctx) {}

Then we can keep the workaround localized to where the compiler bug occurs.

kennyweiss · 2023-01-09T21:01:56Z

I wonder if there is a cleaner workaround that doesn't involve introducing redundant ctors. Would using a function that constructs arg_formatter work? Something along the lines of:
  auto make_arg_formatter(OutputIt iter, format_specs<Char>& s) -> arg_formatter {
    return {iter, s, locale_ref()}
  }

  printf_arg_formatter(OutputIt iter, format_specs<Char>& s, context_type& ctx)
      : base(make_arg_formatter(iter, s), context_(ctx) {}
Then we can keep the workaround localized to where the compiler bug occurs.

Thanks @vitaut
I reworked the branch to convert arg_formatter::specs to a const pointer and added a guarded bugfix for the XL compiler in the printf_arg_formatter constructor.

Would you like me to rework it using the above suggestion?

vitaut · 2023-01-10T18:42:18Z

Would you like me to rework it using the above suggestion?

Yes, please. Using a "factory" function is cleaner because it will allow us to keep the reference while still keeping the workaround localized to printf implementation.

kennyweiss · 2023-01-11T02:48:55Z

Thanks for the suggestion @vitaut -- I think we're getting there!
XL seems happy with the explicit copy constructor for the arg_formatter base class and so does my local gcc-4.8.

vitaut · 2023-01-11T19:28:02Z

include/fmt/format.h

+  static auto make_arg_formatter(iterator iter, format_specs<Char>& s)
+      -> arg_formatter {
+    return {iter, s, locale_ref()};
+  }


Please move this function to printf.h where it is used (it doesn't need to be in arg_formatter) and add a comment that it is a workaround for the XL compiler bug, with a link to the bug report if possible.

Done.

Unfortunately, the IBM ticket is not publicly accessible.

This avoids the need to pass a temporary (default) locale_ref.

…rmatter

Per PR suggestion.

Use this within printf_arg_formatter constructor as workaround for XL compiler bug that optimizes away base class initializer. Also: Reverts conversion of internal `specs` variable back to a const ref Per PR suggestion.

Per PR suggestion.

vitaut · 2023-01-13T19:37:36Z

Merged, thanks. Please comment here once you know which version of the XL compiler fixes the issue.

kennyweiss · 2023-01-13T23:24:31Z

Thanks -- will do.

See: fmtlib/fmt#3256

vitaut requested changes Dec 31, 2022

View reviewed changes

kennyweiss added a commit to LLNL/axom that referenced this pull request Jan 3, 2023

Bugfix to [email protected] for XL compiler

b949135

Upstreamed to fmt: fmtlib/fmt#3256

vitaut requested changes Jan 6, 2023

View reviewed changes

kennyweiss force-pushed the master branch from af34432 to fc91302 Compare January 9, 2023 20:59

kennyweiss force-pushed the master branch from fc91302 to 6348bd6 Compare January 11, 2023 02:45

vitaut reviewed Jan 11, 2023

View reviewed changes

kennyweiss added 8 commits January 12, 2023 17:53

Bugfix for fmt::printf on Power9 architecture with the XL compiler

0a52742

Adds arg_formatter contructor using default locale

ed37406

This avoids the need to pass a temporary (default) locale_ref.

Don't pass locale_ref into arg_formatter constructor in printf_arg_fo…

6cfc4c5

…rmatter

Use a pointer instead of a reference for specs in arg_formatter

10e187d

Per PR suggestion.

Remove constructors for arg_formatter struct

af9a54a

Apply XL compiler bugfix within printf_arg_formatter constructor

e4645d2

Adds a factory function for arg_formatter w/ default locale_ref

4097a24

Use this within printf_arg_formatter constructor as workaround for XL compiler bug that optimizes away base class initializer. Also: Reverts conversion of internal `specs` variable back to a const ref Per PR suggestion.

Moved make_arg_formatter to printf.h

ad7f0c2

Per PR suggestion.

kennyweiss force-pushed the master branch from 6348bd6 to ad7f0c2 Compare January 13, 2023 03:12

vitaut approved these changes Jan 13, 2023

View reviewed changes

vitaut merged commit bfc0924 into fmtlib:master Jan 13, 2023

kennyweiss added a commit to LLNL/axom that referenced this pull request Jan 24, 2023

Updates fmt sprintf patch for XL compiler based on accepted fmt PR

4b5166f

See: fmtlib/fmt#3256

kennyweiss mentioned this pull request Jan 24, 2023

Fixes several outstanding bugs LLNL/axom#1009

Merged

kennyweiss added a commit to LLNL/axom that referenced this pull request Jan 24, 2023

Updates fmt sprintf patch for XL compiler based on accepted fmt PR

b6c3830

See: fmtlib/fmt#3256

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix for fmt::printf on Power9 architecture with the XL compiler #3256

Bugfix for fmt::printf on Power9 architecture with the XL compiler #3256

kennyweiss commented Dec 31, 2022

vitaut left a comment •

edited

Loading

kennyweiss commented Jan 3, 2023

vitaut commented Jan 4, 2023

kennyweiss commented Jan 4, 2023 •

edited

Loading

vitaut Jan 6, 2023

phprus Jan 6, 2023

vitaut Jan 6, 2023 •

edited

Loading

vitaut commented Jan 9, 2023

kennyweiss commented Jan 9, 2023

vitaut commented Jan 10, 2023 •

edited

Loading

kennyweiss commented Jan 11, 2023

vitaut Jan 11, 2023

kennyweiss Jan 13, 2023

vitaut commented Jan 13, 2023

kennyweiss commented Jan 13, 2023

	printf_arg_formatter(OutputIt iter, format_specs<Char>& s, context_type& ctx)
	: base{iter, s, locale_ref()}, context_(ctx) {}

		arg_formatter(buffer_appender<Char> it, const format_specs<Char>& s)
		: out(it), specs(s) {}

Bugfix for fmt::printf on Power9 architecture with the XL compiler #3256

Bugfix for fmt::printf on Power9 architecture with the XL compiler #3256

Conversation

kennyweiss commented Dec 31, 2022

vitaut left a comment • edited Loading

Choose a reason for hiding this comment

kennyweiss commented Jan 3, 2023

vitaut commented Jan 4, 2023

kennyweiss commented Jan 4, 2023 • edited Loading

Description of the error:

vitaut Jan 6, 2023

Choose a reason for hiding this comment

phprus Jan 6, 2023

Choose a reason for hiding this comment

vitaut Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

vitaut commented Jan 9, 2023

kennyweiss commented Jan 9, 2023

vitaut commented Jan 10, 2023 • edited Loading

kennyweiss commented Jan 11, 2023

vitaut Jan 11, 2023

Choose a reason for hiding this comment

kennyweiss Jan 13, 2023

Choose a reason for hiding this comment

vitaut commented Jan 13, 2023

kennyweiss commented Jan 13, 2023

vitaut left a comment •

edited

Loading

kennyweiss commented Jan 4, 2023 •

edited

Loading

vitaut Jan 6, 2023 •

edited

Loading

vitaut commented Jan 10, 2023 •

edited

Loading